Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup builtin plugins interface #5333

Merged
merged 3 commits into from
Jan 2, 2025
Merged

Cleanup builtin plugins interface #5333

merged 3 commits into from
Jan 2, 2025

Conversation

brimoor
Copy link
Contributor

@brimoor brimoor commented Jan 1, 2025

Change log

Code

  • ensure that enable_plugin(), disable_plugin() and delete_plugin() can't be run on builtin plugins
  • consolidate include_builtin and builtins_only arguments into a single builtin argument, for consistency with the existing enabled argument

CLI

  • include builtin column in fiftyone plugins list output
  • include --builtins-only and --no-builtins filtering options for fiftyone plugins|operators list

Docs

  • include plugins API reference in docs build
  • fix various broken links for builtin operators in the docs

Example usage

# Now includes `builtin` column
fiftyone plugins list

# New filtering options
fiftyone plugins list --builtins-only
fiftyone plugins list --no-builtins
fiftyone operators list --builtins-only
fiftyone operators list --no-builtins

# Not allowed
fiftyone plugins enable '@voxel51/operators'
fiftyone plugins disable '@voxel51/operators'
fiftyone plugins delete '@voxel51/operators'

Summary by CodeRabbit

Release Notes

  • Documentation

    • Updated documentation for CLI commands related to operators and plugins
    • Added API reference section for plugins
    • Improved clarity of plugin and operator documentation
    • Updated user guide references for model evaluation
  • New Features

    • Enhanced filtering for operators and plugins (builtin/non-builtin)
    • Added support for more flexible plugin management
    • Introduced example Databricks connector and Data Lens connector
  • Bug Fixes

    • Improved path normalization for plugin directories
    • Refined error handling for built-in plugins
    • Fixed potential StopIteration errors during long-running operations
  • Chores

    • Updated plugin and panel configuration files
    • Streamlined plugin listing and retrieval logic

@brimoor brimoor requested a review from ritch January 1, 2025 17:05
Copy link
Contributor

coderabbitai bot commented Jan 1, 2025

Walkthrough

This pull request introduces comprehensive updates to the FiftyOne documentation generation and plugin management processes. The changes include modifications to documentation generation scripts, CLI command descriptions, plugin documentation, and internal plugin handling mechanisms. Key updates involve expanding API documentation generation, clarifying CLI command behaviors, updating references to operator and plugin paths, and enhancing plugin listing and filtering capabilities.

Changes

File Change Summary
docs/generate_docs.bash Added paths for fiftyone/constants and fiftyone/internal in documentation generation; added new sphinx-apidoc command for plugin API documentation
docs/source/cli/index.rst Updated CLI command descriptions for operators and plugins; added new filtering flags -b, --builtins-only and -c, --no-builtins
docs/source/plugins/developing_plugins.rst Updated module path references for operators from fiftyone.operators.builtin to plugins.operators
docs/source/plugins/index.rst Added new "API reference" entry to table of contents
docs/source/release-notes.rst Updated release notes for FiftyOne Teams version 2.3.0, detailing enhancements and bug fixes
fiftyone/core/cli.py Modified OperatorsListCommand and PluginsListCommand to support new filtering options for builtin/non-builtin items
fiftyone/operators/registry.py Updated list_operators method to use new builtin parameter with more flexible filtering
fiftyone/operators/server.py Adjusted get_operators function to remove include_builtin parameter from operator retrieval
fiftyone/plugins/* Various updates to plugin management functions to support builtin/non-builtin plugin handling
fiftyone/plugins/constants.py Updated path normalization for BUILTIN_PLUGINS_DIR
fiftyone/plugins/context.py Changed argument in build_plugin_contexts function to use builtin="all"
fiftyone/plugins/core.py Renamed include_builtin parameter to builtin in plugin management functions
fiftyone/plugins/definitions.py Updated import statement for constants module
plugins/operators/fiftyone.yml Updated url field to point to the plugins section of the repository
plugins/panels/fiftyone.yml Changed name field from @voxel51/panel to @voxel51/panels
docs/source/user_guide/evaluation.rst Updated reference link in model evaluation documentation
tests/unittests/plugins/core_tests.py Renamed and reversed logic in test functions for enabling and disabling plugins

Suggested labels

enhancement, documentation

Suggested reviewers

  • ritch
  • imanjra

Poem

🐰 Docs dancing, plugins singing bright,
Code paths shifting into light,
Clarity blooms with each new line,
FiftyOne's knowledge starts to shine!
Rabbit hops through documentation's gate 🌈


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
fiftyone/operators/registry.py (1)

138-138: Suggestion to use any(...) for existence checks

You could streamline this loop into a more concise form:

-        for operator in self.list_operators():
-            if operator_uri == operator.uri:
-                return True
-        return False
+        return any(operator_uri == operator.uri for operator in self.list_operators())
🧰 Tools
🪛 Ruff (0.8.2)

138-142: Use return any(operator_uri == operator.uri for operator in self.list_operators()) instead of for loop

Replace with return any(operator_uri == operator.uri for operator in self.list_operators())

(SIM110)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fddb6a4 and 801a489.

📒 Files selected for processing (14)
  • docs/generate_docs.bash (1 hunks)
  • docs/source/cli/index.rst (6 hunks)
  • docs/source/plugins/developing_plugins.rst (3 hunks)
  • docs/source/plugins/index.rst (1 hunks)
  • docs/source/release-notes.rst (1 hunks)
  • fiftyone/core/cli.py (11 hunks)
  • fiftyone/operators/registry.py (4 hunks)
  • fiftyone/operators/server.py (1 hunks)
  • fiftyone/plugins/constants.py (1 hunks)
  • fiftyone/plugins/context.py (3 hunks)
  • fiftyone/plugins/core.py (11 hunks)
  • fiftyone/plugins/definitions.py (2 hunks)
  • plugins/operators/fiftyone.yml (1 hunks)
  • plugins/panels/fiftyone.yml (1 hunks)
✅ Files skipped from review due to trivial changes (4)
  • fiftyone/plugins/constants.py
  • plugins/panels/fiftyone.yml
  • plugins/operators/fiftyone.yml
  • fiftyone/plugins/definitions.py
🧰 Additional context used
🪛 Ruff (0.8.2)
fiftyone/operators/registry.py

138-142: Use return any(operator_uri == operator.uri for operator in self.list_operators()) instead of for loop

Replace with return any(operator_uri == operator.uri for operator in self.list_operators())

(SIM110)

🔇 Additional comments (40)
fiftyone/plugins/core.py (11)

52-71: Consolidated include_builtin and builtins_only parameters into builtin

This new parameter logic is clear and aligns with the objective to unify how built-in plugins are handled. The approach of setting builtin = None for the "all" case is consistent and easy to follow.


86-92: Prevent enabling builtin plugins

Raising a ValueError when attempting to enable a builtin plugin clearly enforces the new restriction. The implementation and error message are straightforward and informative.


101-107: Prevent disabling builtin plugins

Like enabling, the code properly raises a ValueError when users try to disable builtin plugins, maintaining consistency with the intended restrictions.


116-121: Prevent deleting builtin plugins

This code segment correctly prohibits deleting builtin plugins, complementing the prior checks for enabling/disabling. The error message is concise and helpful.


130-130: Returning non-builtin plugins only

Changing return _list_plugins_by_name(builtin=False) in list_downloaded_plugins() is a clear way to skip builtin plugins. This clarifies the function’s intended scope.


139-139: Exclude builtin plugins from list_enabled_plugins()

Specifying builtin=False while returning enabled plugins is consistent with the method name and new design.


148-148: Exclude builtin plugins from list_disabled_plugins()

Consistently uses the new builtin=False parameter for disabled plugin listing, ensuring builtin plugins are ignored.


529-539: Selectively load builtin or non-builtin plugins

The branching on builtin in (True, False, None) to load either built-in, non-built-in, or both types of plugin metadata is clear and maintains code readability.


541-541: Iterating plugin paths based on builtin filter

This loop flows naturally from the plugin path collection logic above. No further issues noted.


560-565: Propagate builtin filter to _list_plugins_by_name()

Passing the new builtin parameter to _list_plugins() ensures consistent filtering logic by name. The changes are straightforward and functionally correct.


610-612: Add builtin filter to _get_plugin()

Allowing _get_plugin to filter by builtin is aligned with the rest of the updated plugin management methods. Implementation looks good.

fiftyone/plugins/context.py (3)

38-38: Use builtin="all" in build_plugin_contexts()

Switching from include_builtin=True to builtin="all" matches the new unified parameter semantics, ensuring all plugins are loaded.


66-68: Enhanced docstring for secrets property

Rewriting the docstring in a multi-line format improves readability and clarity for users referencing this property.


101-102: Improved docstring formatting for register() method

Splitting the docstring lines clearly differentiates operators from panels, making this reference more user-friendly.

fiftyone/operators/registry.py (3)

35-53: Introduce builtin="all" in list_operators()

Renaming builtins_only to builtin with a three-state logic (True, False, or "all") achieves consistency with plugin filtering. The docstring changes are thorough and clear.


84-89: Add builtin parameter to OperatorRegistry.list_operators()

The updated signature and docstring align well with the overarching changes, making it easy to filter for built-in or non-built-in operators.


101-104: Filter operators by builtin status

This conditional check for builtin is True or False is clearly implemented and complements the rest of the codebase.

fiftyone/operators/server.py (1)

30-31: Removing include_builtin usage in get_operators()

Relying on type="operator" and type="panel" only is correct under the new builtin logic. This simplifies the collection process without affecting the user’s experience.

fiftyone/core/cli.py (12)

Line range hint 2908-2924: Docstring updates for OperatorsListCommand

Documenting the use cases for --builtins-only and --no-builtins clarifies the new filtering options for operators and panels.


2944-2957: Add new flags for builtin filtering in OperatorsListCommand

Introducing --builtins-only and --no-builtins provides a clear, intuitive way for users to filter operators by builtin status.


2988-2994: Set builtin argument based on --builtins-only / --no-builtins

Clearly handles user inputs for specifying True, False, or "all". This logic is concise and maintainable.


3002-3002: Print updated list of operators

Passing enabled, builtin, type to _print_operators_list is consistent with the new filtering approach.


3005-3006: Updated _print_operators_list

Retrieving operators via foo.list_operators(enabled=enabled, builtin=builtin, type=type) seamlessly accommodates the new CLI flags.


3043-3043: Docstring update for OperatorsInfoCommand

Small but clear docstring improvement, ensuring consistency with plugin-related updates.


Line range hint 3525-3539: Docstring changes for PluginsListCommand

Explicitly referencing --no-builtins helps users discover the new builtin filtering behavior. The examples are informative.


3558-3571: CLI flags for listing builtin or non-builtin plugins

Similar to the operators command, adding --builtins-only and --no-builtins arguments is a straightforward and user-friendly approach.


3588-3594: Determine builtin parameter

Assigning True, False, or "all" based on user input is consistent with the rest of the code changes.


3598-3599: Pass new builtin filter to _print_plugins_list()

Allowing the CLI to control whether builtin plugins are displayed complements the changes in the plugin core code.


3609-3616: Conditionally mark plugins as enabled if builtin

Automatically marking builtin plugins as enabled in the table is intuitive, preventing confusion about their locked status.

Also applies to: 3617-3617


3629-3629: Docstring refinement for PluginsInfoCommand

These changes keep the plugins info command aligned with the rest of the revised plugin management approach.

docs/generate_docs.bash (2)

106-107: LGTM: Including additional modules for documentation

The addition of fiftyone/constants and fiftyone/internal modules to the documentation generation improves API coverage and transparency.


113-116: LGTM: Adding plugin API documentation generation

The addition of a dedicated sphinx-apidoc command for plugins documentation enhances the documentation structure and aligns with the PR's goal of improving plugin documentation.

docs/source/plugins/index.rst (1)

131-131: LGTM: Adding plugin API reference to documentation structure

The addition of the API reference to the table of contents improves documentation navigation and complements the new plugin API documentation generation.

docs/source/cli/index.rst (3)

833-834: LGTM: Enhanced operator CLI documentation

The updates to operator-related CLI documentation:

  • Clarify that commands work with locally installed operators
  • Add new flags (--builtins-only, --no-builtins) for filtering builtin operators
  • Include examples demonstrating the new filtering capabilities

These changes align well with the PR's objective to improve the builtin plugins interface.

Also applies to: 841-841, 855-856, 880-885, 893-893


1179-1180: LGTM: Improved plugin command descriptions

The updated descriptions clearly indicate that the commands work with locally installed plugins, maintaining consistency with the operator command documentation.


1193-1193: LGTM: Enhanced plugin CLI documentation

The updates to plugin-related CLI documentation:

  • Clarify the local scope of commands
  • Add new flags (--builtins-only, --no-builtins) for filtering builtin plugins
  • Include examples demonstrating the new filtering capabilities

These changes maintain consistency with operator documentation and support the PR's goals.

Also applies to: 1204-1209, 1228-1232, 1238-1238

docs/source/plugins/developing_plugins.rst (2)

678-678: Documentation path reference updated consistently

The module path reference has been updated consistently with the earlier change, maintaining documentation coherence.


88-88: Documentation path reference updated correctly

The module path reference has been updated from fiftyone.operators.builtin.operations to plugins.operators, which aligns with the restructured plugin architecture.

Let's verify the consistency of this path reference across the documentation:

✅ Verification successful

Module path reference update is consistent across documentation

Based on the search results, the module path reference change from fiftyone.operators.builtin.operations to plugins.operators is consistent across the documentation. The new path plugins.operators is correctly used in multiple places including release notes and plugin documentation, while there are no remaining references to the old path fiftyone.operators.builtin.operations. The change aligns with the broader restructuring of the plugin architecture.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining old path references
rg "fiftyone\.operators\.builtin\.operations" --type rst

Length of output: 58


Script:

#!/bin/bash
# Let's check for both old and new module references to ensure consistency
echo "Checking for old module path patterns:"
rg "fiftyone\.operators\.builtin" --type rst

echo -e "\nChecking for new module path patterns:"
rg "plugins\.operators" --type rst

echo -e "\nChecking for any .rst files in plugins documentation:"
fd -e rst -p "**/plugins/**"

Length of output: 1341


Script:

#!/bin/bash
# Let's check for .rst files in plugins directory with proper fd syntax
echo "Checking documentation files in plugins directory:"
fd -e rst . docs/source/plugins/

# Let's also check for any remaining references to operators in the RST files
echo -e "\nChecking for operator references in documentation:"
rg -A 2 -B 2 "operators" --type rst

Length of output: 100475

docs/source/release-notes.rst (2)

166-172: LGTM: Updated operator import paths

The changes correctly update the documentation to reflect the new import paths for the CreateIndex, ClearSampleField, and ClearFrameField operators from fiftyone.operators.builtin to plugins.operators.


Line range hint 1-1000: LGTM: Well-structured release notes

The release notes are well-organized and provide clear, detailed information about changes, enhancements, and bug fixes for each version. The content follows proper documentation format and style.

@brimoor brimoor force-pushed the builtin-plugins-cleanup branch from 801a489 to 6bbc7fe Compare January 1, 2025 18:40
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
tests/unittests/plugins/core_tests.py (2)

53-57: Use simpler conditionals when checking for false

Instead of comparing to False, consider using a negation (if not ...) for improved readability, e.g.:

-assert config["plugins"]["my-plugin"]["enabled"] == False
+assert not config["plugins"]["my-plugin"]["enabled"]
🧰 Tools
🪛 Ruff (0.8.2)

57-57: Avoid equality comparisons to False; use if not config["plugins"]["my-plugin"]["enabled"]: for false checks

Replace with not config["plugins"]["my-plugin"]["enabled"]

(E712)


60-65: Avoid explicit equality check with True

Instead of ... == True, use a direct truth check for improved readability, e.g.:

-assert config["plugins"].get("my-plugin", {}).get("enabled", True) == True
+assert config["plugins"].get("my-plugin", {}).get("enabled", True)
🧰 Tools
🪛 Ruff (0.8.2)

65-65: Avoid equality comparisons to True; use if ...: for truth checks

Replace comparison

(E712)

fiftyone/operators/registry.py (1)

Line range hint 138-142: Refactor operator existence check

Consider simplifying the loop into a one-liner:

-def operator_exists(self, operator_uri):
-    for operator in self.list_operators():
-        if operator_uri == operator.uri:
-            return True
-
-    return False
+def operator_exists(self, operator_uri):
+    return any(operator_uri == op.uri for op in self.list_operators())
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 801a489 and 6bbc7fe.

📒 Files selected for processing (16)
  • docs/generate_docs.bash (1 hunks)
  • docs/source/cli/index.rst (6 hunks)
  • docs/source/plugins/developing_plugins.rst (3 hunks)
  • docs/source/plugins/index.rst (1 hunks)
  • docs/source/release-notes.rst (1 hunks)
  • docs/source/user_guide/evaluation.rst (1 hunks)
  • fiftyone/core/cli.py (11 hunks)
  • fiftyone/operators/registry.py (4 hunks)
  • fiftyone/operators/server.py (1 hunks)
  • fiftyone/plugins/constants.py (1 hunks)
  • fiftyone/plugins/context.py (3 hunks)
  • fiftyone/plugins/core.py (10 hunks)
  • fiftyone/plugins/definitions.py (2 hunks)
  • plugins/operators/fiftyone.yml (1 hunks)
  • plugins/panels/fiftyone.yml (1 hunks)
  • tests/unittests/plugins/core_tests.py (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • docs/source/user_guide/evaluation.rst
🚧 Files skipped from review as they are similar to previous changes (12)
  • docs/source/plugins/index.rst
  • plugins/panels/fiftyone.yml
  • docs/generate_docs.bash
  • fiftyone/operators/server.py
  • plugins/operators/fiftyone.yml
  • fiftyone/plugins/constants.py
  • fiftyone/plugins/context.py
  • fiftyone/core/cli.py
  • fiftyone/plugins/definitions.py
  • docs/source/cli/index.rst
  • docs/source/plugins/developing_plugins.rst
  • docs/source/release-notes.rst
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/unittests/plugins/core_tests.py

57-57: Avoid equality comparisons to False; use if not config["plugins"]["my-plugin"]["enabled"]: for false checks

Replace with not config["plugins"]["my-plugin"]["enabled"]

(E712)


65-65: Avoid equality comparisons to True; use if ...: for truth checks

Replace comparison

(E712)

fiftyone/operators/registry.py

138-142: Use return any(operator_uri == operator.uri for operator in self.list_operators()) instead of for loop

Replace with return any(operator_uri == operator.uri for operator in self.list_operators())

(SIM110)

🔇 Additional comments (22)
fiftyone/operators/registry.py (5)

35-42: New builtin parameter design

This updated function signature (def list_operators(enabled=True, builtin="all", type=None)) with a single builtin parameter is a neat unification of prior flags. It simplifies consistent usage across the codebase.


49-50: Clear and concise conversion from "all" to None

Converting builtin == "all" to None is a good approach to unify logic internally.


53-53: Passing builtin to the registry

Propagation of the new builtin parameter to the registry is consistent with the updated design. Looks good.


84-89: Parameter docstrings alignment

The docstring now clearly explains the new builtin parameter. Ensure that the default value/behavior for builtin matches both code and docstring to avoid confusion.


101-104: Builtin filter logic is correct

Filtering operators based on _builtin is True or _builtin is False is logically sound.

fiftyone/plugins/core.py (17)

52-59: Unified builtin parameter

Replacing include_builtin and builtins_only with a single builtin argument is clearer and simpler. Good improvement.


67-69: Use of 'all' sentinel

Converting 'all'None aligns with similar usage in other modules. This helps maintain consistent filtering logic.


71-71: Efficient plugin loading

Looping over _list_plugins(...) once to collect plugin definitions is straightforward and easy to follow.


80-80: Plugin enable function signature clarity

Adding _allow_missing=False as a default is helpful to control how strictly missing plugins are treated.


86-98: Restrict changes on builtin plugins

Raising an exception when a user attempts to enable a builtin plugin is a robust safeguard. This enforces the read-only nature of builtin plugins.


102-102: Graceful plugin disable function

Mirroring the _allow_missing behavior for disabling is consistent and symmetrical with enabling logic.


108-120: Prevent builtin plugin disable

Throwing an error if the plugin is builtin preserves its read-only state. Good approach.


130-133: Integrity protection for builtin plugins

Raising a ValueError for deleting builtin plugins fortifies plugin integrity.


135-135: Ensure plugin cleanup

Deletion of the plugin directory with etau.delete_dir aligns with user expectations.


144-144: Listing downloaded plugins

list_downloaded_plugins() now excludes builtin plugins. Good approach to keep them separate from user-installed ones.


153-153: Restrict user plugin enablement list

list_enabled_plugins() ensures only user plugins (non-builtin) are returned.


162-162: Disable plugin enumeration

list_disabled_plugins() clearly scopes to non-builtin plugins for consistent user-facing behavior.


543-553: Collecting builtin vs. non-builtin plugin paths

Reading plugin metadata from both builtin and user plugin locations in _list_plugins(...) is properly toggled by the builtin argument.


555-555: Graceful plugin loading with exceptions

The try/except block around _parse_plugin_metadata ensures that failing plugins don’t halt the entire enumeration.


574-579: Clearer function name _list_plugins_by_name

Filtering logic for enabled and builtin in _list_plugins_by_name remains consistent with _list_plugins. Good job.


624-626: _get_plugin: searching by name

This chunk reuses _list_plugins(...) with potential duplicates check. The approach is well-structured.


730-730: Simplify removing default-enabled state

Removing "enabled" from plugin settings when set to True is an elegant way to handle the default-enabled scenario.

rows = []
for pd in plugin_defintions:
rows.append(
{
"plugin": pd.name,
"version": pd.version or "",
"enabled": pd.name in enabled_plugins,
"enabled": pd.builtin or pd.name in enabled_plugins,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 👍

plugin = _get_plugin(plugin_name)
plugin = get_plugin(plugin_name)
if plugin.builtin:
raise ValueError(f"Cannot delete builtin plugin '{plugin_name}'")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
fiftyone/plugins/core.py (2)

102-120: Consider consolidating duplicate protection logic

The builtin protection logic is duplicated across enable_plugin and disable_plugin functions. Consider extracting this into a helper function.

+def _ensure_not_builtin(plugin, operation):
+    if plugin is not None and plugin.builtin:
+        raise ValueError(
+            f"Cannot {operation} builtin plugin '{plugin.name}'"
+        )
+
 def enable_plugin(plugin_name, _allow_missing=False):
     try:
         plugin = get_plugin(plugin_name)
     except ValueError:
         if not _allow_missing:
             raise
         plugin = None
-    if plugin is not None and plugin.builtin:
-        raise ValueError(
-            f"Cannot change enablement of builtin plugin '{plugin_name}'"
-        )
+    _ensure_not_builtin(plugin, "change enablement of")

730-730: Consider adding a comment explaining the default enablement behavior

The code removes the "enabled" entry to indicate default enablement, but this behavior isn't immediately obvious.

+            # Remove the "enabled" entry since True is the default state
             plugin_settings.pop("enabled", None)
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6bbc7fe and 75c481b.

📒 Files selected for processing (1)
  • fiftyone/plugins/core.py (11 hunks)
🔇 Additional comments (4)
fiftyone/plugins/core.py (4)

52-71: LGTM: Clean parameter consolidation for plugin filtering

The consolidation of include_builtin into a three-state builtin parameter aligns well with the existing enabled parameter pattern, improving API consistency.


130-133: LGTM: Consistent protection for builtin plugins

The delete_plugin function correctly prevents deletion of builtin plugins, maintaining consistency with enable/disable operations.


543-553: LGTM: Clean implementation of plugin listing

The _list_plugins implementation efficiently handles both builtin and non-builtin plugins with clear conditional logic.


80-98: Verify error handling for builtin plugin operations

The protection logic for builtin plugins is sound, but let's verify the error handling across the codebase.

✅ Verification successful

Error handling for builtin plugins is properly implemented

The verification shows that:

  • Builtin plugin protection is consistently implemented across the codebase
  • Direct calls to enable_plugin() are properly handled in:
    • CLI commands (fiftyone/core/cli.py)
    • Test cases (tests/unittests/plugins/core_tests.py)
  • The builtin plugin check is also consistently applied in related operations:
    • Plugin disablement (fiftyone/plugins/core.py)
    • Plugin deletion (fiftyone/plugins/core.py)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for direct calls to enable_plugin to ensure proper error handling
rg -A 3 "enable_plugin\(" --type python

# Search for potential error handling patterns
ast-grep --pattern 'try {
  $$$
  enable_plugin($$$)
  $$$
}'

Length of output: 136


Script:

#!/bin/bash
# Search for direct calls to enable_plugin with proper file extension
rg -A 3 "enable_plugin\(" --type py

# Search for error handling patterns around enable_plugin
rg -B 2 -A 3 "try:.*enable_plugin" --type py

# Look for other references to builtin plugins
rg "builtin.*plugin" --type py

Length of output: 2272

@brimoor brimoor requested a review from ritch January 2, 2025 15:00
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
fiftyone/operators/registry.py (1)

Line range hint 138-142: Consider using any() for a more concise implementation.

The current implementation can be simplified using Python's built-in any() function for better readability and potentially better performance.

-        for operator in self.list_operators():
-            if operator_uri == operator.uri:
-                return True
-
-        return False
+        return any(operator_uri == operator.uri for operator in self.list_operators())
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 75c481b and c8c4ef5.

📒 Files selected for processing (1)
  • fiftyone/operators/registry.py (4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
fiftyone/operators/registry.py

138-142: Use return any(operator_uri == operator.uri for operator in self.list_operators()) instead of for loop

Replace with return any(operator_uri == operator.uri for operator in self.list_operators())

(SIM110)

🔇 Additional comments (2)
fiftyone/operators/registry.py (2)

35-42: Clean and consistent parameter naming!

The change from builtins_only to builtin with values True/False/"all" makes the API more consistent and intuitive. The docstring accurately reflects the changes and addresses the previous documentation confusion.


49-51: Clean implementation of builtin filtering logic!

The conversion of "all" to None and the subsequent filtering logic is clear and efficient. This change aligns well with the PR's objective of improving the builtin plugins interface.

Also applies to: 101-104

fiftyone/server \
fiftyone/service \
fiftyone/management \
fiftyone/api

sphinx-apidoc --force --no-toc --separate --follow-links \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see...

image

Hmm we haven't really figured out how to document operators/panels in a way that they provide utility in our docs.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it makes sense to add this as "API Reference" for plugins. I think the existing fiftyone.operators.operations is what I would expect in a section like that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also is there any way to adjust the header/breadcrumb - it confusingly says "Docs / Plugins Overview / plugins"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it's not very usable in its current form. The only reason I added this is that #5261 broke a few links in the plugins docs and release notes that used to link to (equally unhelpful) resources about the available builtin operators in the fiftyone API reference tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One possible solution is to add class docstrings to all operators/panels that describe how to use them, like I did for ViewExpression here:
https://docs.voxel51.com/api/fiftyone.core.expressions.html?highlight=viewexpression#fiftyone.core.expressions.ViewExpression

That would be a bit awkward though for this use case.

It's not that useful to have the panels in the API reference, as they aren't meant to be used programmatically and really should already have dedicated documentation sections for them.

There is some value in having a list of all the builtin operators, in the sense that they are totally undocumented to-date. It would be reasonable for some of them to have programmatic execution examples, but generally they are really intended to be invoked via the App's Operator browser. But a docstring is an awkward place to document in-App usage. What we really need is some kind of operator documentation page that shows GIFs of every single operator in use.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we really need is some kind of operator documentation page that shows GIFs of every single operator in use.

Sure - I've always thought that we could allow for self-documentation, since operators define their own input/output.

We should change the "API reference". Based on what you said above I think the only value of this is the listing of what is in /plugins. In that case we should call it "Built-in plugins" or something similar.

Copy link
Contributor

@ritch ritch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should look at how the new builtin plugins are displayed in the docs. Otherwise LGTM.

@brimoor brimoor merged commit 86eb964 into develop Jan 2, 2025
14 checks passed
@brimoor brimoor deleted the builtin-plugins-cleanup branch January 2, 2025 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants